fix: preserve sandbox recovery readiness guard - #7291
Conversation
Signed-off-by: ScarabSystems <scarab.systems@yahoo.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughSandbox recovery preserves tri-state managed-health results during recreated-sandbox readiness checks, retries inconclusive outcomes within the deadline, fails closed on definitive failures, and reports categorized forward-recovery failure details. ChangesSandbox recovery readiness
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Recovery
participant ManagedHealth
participant OpenShell
Recovery->>ManagedHealth: Confirm relaunched supervisor health
ManagedHealth-->>Recovery: Ready, failed, or inconclusive
Recovery->>ManagedHealth: Retry inconclusive probe within deadline
Recovery->>OpenShell: Capture readiness when managed guard passes
OpenShell-->>Recovery: OpenShell readiness result
Recovery-->>Recovery: Set forward recovery status and failure detail
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. Since last review: 0 prior items resolved · 0 still apply · 0 new items found E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/actions/sandbox/process-recovery.ts (1)
1198-1203: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReference-equality dispatch on
waitForRecreatedSandboxOpenShellReadyImplis fragile.Branching on
waitForRecreatedSandboxOpenShellReadyImpl === waitForRecreatedSandboxOpenShellReadyto decide whether categorized failure info is available silently degrades to a generic"openshell-readiness-failure"whenever any caller supplies a boolean-returning override (even a legitimate wrapper/logging shim around the same function), since identity is lost. Consider exposing an injectable option that returns the structuredRecreatedSandboxOpenShellReadinessResultdirectly (e.g. awaitForRecreatedSandboxOpenShellReadyResultImploption) instead of keying behavior off function identity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/sandbox/process-recovery.ts` around lines 1198 - 1203, Replace the reference-equality dispatch in the readiness flow with an explicit injectable structured-result implementation, such as waitForRecreatedSandboxOpenShellReadyResultImpl. Use that option when provided, otherwise retain the boolean-returning waitForRecreatedSandboxOpenShellReadyImpl path and its generic failure result; do not infer behavior from function identity.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/actions/sandbox/process-recovery.ts`:
- Around line 1192-1216: Update confirmRelaunchedManagedHealth and its relaunch
beforeProbe path to preserve the tri-state boolean | null result instead of
coercing it to boolean. Ensure waitForRecreatedSandboxOpenShellReadyResult can
receive null and execute its inconclusive-retry branch, while retaining boolean
coercion only at callers that explicitly require a boolean.
---
Nitpick comments:
In `@src/lib/actions/sandbox/process-recovery.ts`:
- Around line 1198-1203: Replace the reference-equality dispatch in the
readiness flow with an explicit injectable structured-result implementation,
such as waitForRecreatedSandboxOpenShellReadyResultImpl. Use that option when
provided, otherwise retain the boolean-returning
waitForRecreatedSandboxOpenShellReadyImpl path and its generic failure result;
do not infer behavior from function identity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b5d32f4b-8a31-40ff-8a71-e6508372061e
📒 Files selected for processing (3)
src/lib/actions/sandbox/process-recovery.test.tssrc/lib/actions/sandbox/process-recovery.tstest/process-recovery-supervisor-relaunch.test.ts
Signed-off-by: ScarabSystems <scarab.systems@yahoo.com>
Signed-off-by: ScarabSystems <scarab.systems@yahoo.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/process-recovery-supervisor-relaunch.test.ts (1)
283-283: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid locking the test to the exact probe count.
The busy-then-success sequence only needs to prove that the busy response was retried. Use a minimum sufficient count instead of exactly
5, so harmless polling changes do not break this regression test.Proposed change
- expect(requestPinnedGatewaySupervisorAction).toHaveBeenCalledTimes(5); + expect(requestPinnedGatewaySupervisorAction.mock.calls.length).toBeGreaterThanOrEqual(3);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/process-recovery-supervisor-relaunch.test.ts` at line 283, Update the assertion for requestPinnedGatewaySupervisorAction in the busy-then-success recovery test to verify a minimum sufficient invocation count rather than exactly five calls, while preserving coverage that the busy response was retried.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/process-recovery-supervisor-relaunch.test.ts`:
- Line 283: Update the assertion for requestPinnedGatewaySupervisorAction in the
busy-then-success recovery test to verify a minimum sufficient invocation count
rather than exactly five calls, while preserving coverage that the busy response
was retried.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6e1a91a6-84fe-4258-9bc8-04be8d21bc91
📒 Files selected for processing (2)
src/lib/actions/sandbox/process-recovery.tstest/process-recovery-supervisor-relaunch.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/actions/sandbox/process-recovery.ts
|
✨ Thanks for the fix. This addresses a real sandbox recovery reliability gap where a healthy recreated gateway could be falsely failed. The managed-health result preservation with bounded retry and fail-closed semantics looks well-scoped. Related open issues: Related open issues: |
<!-- markdownlint-disable MD041 --> ## Summary `nemoclaw <sandbox> stop` returned success but never tore down the host-side dashboard SSH port-forward it created, leaving an `ssh -L 127.0.0.1:<port>` listener alive. `status` then misreported the cleanly-stopped sandbox as a foreign `sandbox_dashboard_port_conflict`, and `start`/`recover` intermittently failed while contending for the still-held port. This PR releases the forward on stop. Exact-head live `double-onboard` also exposed the known recreated-sandbox readiness defect from #7273, so this branch includes the reviewed tri-state readiness fix from #7291 and keeps the restart regression enabled. Closes #7227. Closes #7273. ## Reproduction On our Ubuntu 24.04 x86_64 test host (no GPU), ollama-local sandbox. The reporter is on DGX Spark aarch64; the root cause (host-side port-forward not torn down on stop) is platform-independent. 1. Onboard an OpenClaw sandbox on ollama-local; confirm `Phase: Ready`. 2. `openshell forward list` / `ss -ltnp | grep <dashboard-port>` — a nemoclaw-spawned `ssh -L` forward holds the port. 3. `nemoclaw <sb> stop` (exit 0; container Exited 137). 4. Re-check the port and `nemoclaw <sb> status`. **Observed on `main` (before fix)** ``` # after stop, the forward is still alive: $ ss -ltnp | grep 18789 LISTEN 127.0.0.1:18789 ... users:(("ssh",pid=<same as before stop>,fd=3)) $ openshell forward list <sb> 127.0.0.1 18789 <pid> running $ nemoclaw <sb> status # EXIT 1 Failure layer: sandbox_dashboard_port_conflict — sandbox container is stopped and the dashboard port is held by a foreign listener. ``` **Observed on `fix/...` (after fix)** ``` # after stop, no listener remains: $ ss -ltnp | grep 18789 # 0 listeners $ openshell forward list # 0 entries for <sb> $ nemoclaw <sb> status Failure layer: sandbox_container_stopped — sandbox container exists but is not running. # repeated start/stop cycles (x3): start always reaches Ready; stop always leaves 0 listeners. ``` ## Analysis `stopSandbox` (`src/lib/actions/sandbox/stop.ts`) stops the labeled container(s) and returns, but it never releases the host-side dashboard port-forward. That forward is a nemoclaw-managed `openshell forward` (`ssh -L 127.0.0.1:<port>`); onboarding, destroy, and forward-recovery all tear it down with `openshell forward stop <port> <sandbox>`, but `stop` did not — so the listener outlives the container. `getSandboxGatewayStateForStatus`'s classifier (`gateway-failure-classifier.ts`, `sandbox_dashboard_port_conflict`) then sees a stopped container plus a live listener on its dashboard port and reports a "foreign listener" conflict — the listener is nemoclaw's own leftover forward. `start`/`recover` re-establish the forward and probe gateway readiness while the just-stopped forward is still bound, which is the intermittent failure. The exact-head E2E gate also exposed a finalization ordering gap: `cloud-onboard` created the sandbox and launched OpenClaw, but post-policy process recovery invalidated the initial dashboard forward before final deployment verification. Finalization re-established forwards only for non-default agents and did so before recovery, so the delivered OpenClaw dashboard on `127.0.0.1:18789` returned connection refused. Existing unit review did not model recovery invalidating the forward; the new regression makes that state transition explicit. ## Fix After the container is stopped, release the forward via a new `teardownSandboxDashboardForward` helper (`forward-recovery.ts`) that runs `openshell forward stop <port> <sandbox>` — reusing the exact teardown already used by onboard/destroy/forward-recovery, with the port from `resolveSandboxDashboardPort`. The helper scopes cleanup to the registry-owned gateway, applies the standard 30-second OpenShell operation timeout, and waits up to 5 seconds for the supported OpenShell 0.0.85 listener to release the host port. It is best-effort (a stop must still free container resources when OpenShell is unreachable) and is wired into `stopSandbox` after a successful Docker stop and for an already-stopped sandbox. It does not run when Docker stop fails because the container is still running and needs its forward. Tests (`stop.test.ts`) pin the contract: the forward is torn down only after the container stops; it is not released when Docker stop fails; and a repeated stop releases a leftover forward for an already-at-rest sandbox. They also verify non-default gateway targeting, the operation timeout, delayed listener-release polling, and that a nonzero cleanup status skips the port probe. Finalization now reconciles the default OpenClaw or declared-agent forward after post-policy process recovery and before warm-up or deployment verification. The regression simulates recovery invalidating an initially-live forward and requires finalization to restore it before reporting the deployment healthy. ## Changes - `src/lib/actions/sandbox/process-recovery.ts` and focused tests: preserve the reviewed #7291 tri-state readiness contract and retry only the exact OpenShell supervisor re-registration states and no-op readiness-probe timeout exposed by exact-head `double-onboard`. - `test/e2e/live/double-onboard.test.ts`: retain the stop/start regression so live validation proves the recovery dependency instead of bypassing it. - `src/lib/actions/sandbox/forward-recovery.ts`: new `teardownSandboxDashboardForward` helper. - `src/lib/actions/sandbox/stop.ts`: release the dashboard forward after a successful stop. - `src/lib/actions/sandbox/stop.test.ts`: pin the teardown contract and its boundaries. - `src/lib/onboard.ts` and finalization tests: restore dashboard forwarding after post-policy process recovery, including the default OpenClaw path. - `src/lib/onboard/policy-selection.ts` and focused tests: after applying policy presets, require both sandbox phase readiness and a real OpenShell command probe before finalization. - Timeout documentation: map post-policy OpenShell re-registration to `NEMOCLAW_SANDBOX_READY_TIMEOUT` and distinguish it from post-create cleanup. - `docs/manage-sandboxes/run-sandboxes.mdx`: document dashboard-forward cleanup during stop. - `docs/reference/commands.mdx`: document cleanup ordering, failure handling, and idempotency. ## Type of Change - [x] Code change (feature, bug fix, or refactor) ## Verification - [x] Focused recovery tests pass: 32/32. - [x] Focused finalization tests pass: 16/16. - [x] Exact affected Vitest suite against current `main` passes: 107 files / 1,060 tests. - [x] CLI type-check, CLI build, Biome, semantic E2E plans, `git diff --check`, normal pre-commit/commit-msg hooks, pre-push hooks, and gitleaks pass. - [x] `npm run docs` passes with 0 errors and 2 existing warnings; agent variants are synchronized and valid. - [ ] Replacement exact-head live E2E is pending for the OpenShell relay-readiness and final-forward ordering fixes. - [x] No secrets, API keys, or credentials committed ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: PASS on exact head `f7acc8049d1cccb914cbb7328bbfcd4deb378814` against base `04e6dfa883071dda9df429c66e73168e1a995cba`. The reviewer inspected the full 22-file PR diff, including both exact OpenShell 0.0.85 relay-readiness timeout forms, refreshed-main resolution, final recovery/forward reconciliation, and all agent variants. Existing documentation sufficiently covers the bounded readiness, fail-closed recovery, forward cleanup, and final verification behavior. `npm run docs`, agent-variant validation, and `git diff --check` passed. - Agent: `Codex Desktop` <!-- docs-review-head-sha: f7acc80 --> <!-- docs-review-agents-blob-sha: 9c9b36d --> ## AI Disclosure - [x] AI-assisted — tool: Claude Code Signed-off-by: Yanyun Liao <yanyunl@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved sandbox stop to best-effort release the host dashboard port-forward, including idempotent cleanup and safer degradation when OpenShell isn’t available. * Strengthened recovery readiness handling with bounded, per-stage timeouts, clearer retry vs fail-closed outcomes, and better gating before starting port-forwards. * Updated onboarding finalization to restore the correct dashboard forward after recovery and to gate preset synchronization on control-plane readiness. * **Tests** * Expanded unit, supervisor relaunch, onboarding, and e2e coverage for forward release, readiness retries, and edge-case recovery flows. * **Documentation** * Refreshed stop/recovery and timeout guidance to match the preserved assets and the updated dashboard forward behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Yanyun Liao <yanyunl@nvidia.com> Signed-off-by: ScarabSystems <scarab.systems@yahoo.com> Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: ScarabSystems <scarab.systems@yahoo.com> Co-authored-by: Charan Jagwani <cjagwani@nvidia.com> Co-authored-by: Prekshi Vyas <34834085+prekshivyas@users.noreply.github.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com>
|
Closing this rather than rebasing a conflict-resolution update over current main. The same sandbox recovery readiness surface has since moved forward through #7509, which is merged on main and carries the current OpenShell re-registration handling for this path. Keeping this PR open would add another review pass across the same three files without a clearer incremental change, so closing to avoid review noise. |
Record NVIDIA/NemoClaw#7406 as an accepted #7387 repair after upstream merge. Refresh NVIDIA/NemoClaw#7254 as accepted test coverage for #6042 while noting the issue remains open and the merged PR was test-only. Refresh NVIDIA/NemoClaw#7291/#7273 as closed without merge so the Field Lab no longer reports that PR as open. Verification: gh pr/issue views for #7254, #7406, #7291, #6042, #7387, and #7273; git diff --cached --check; public leakage scan for local/private/SDS terms.
Summary
Fixes #7273.
This change preserves the
true | false | nullmanaged-health result through the recreated-sandbox OpenShell readiness guard.The change does not accept broader OpenShell errors and does not substitute direct Docker health for control-plane readiness.
Validation
npm cinpx vitest run --project cli src/lib/actions/sandbox/process-recovery.test.ts test/process-recovery-supervisor-relaunch.test.tsHOME=/workspace/workspace-nemoclaw-7273-recovery-readiness-20260721-01/cache/test-home npx vitest run --project integration test/process-recovery-supervisor-relaunch.test.tsnpx @biomejs/biome format src/lib/actions/sandbox/process-recovery.ts src/lib/actions/sandbox/process-recovery.test.ts test/process-recovery-supervisor-relaunch.test.tsnpx @biomejs/biome lint src/lib/actions/sandbox/process-recovery.ts src/lib/actions/sandbox/process-recovery.test.ts test/process-recovery-supervisor-relaunch.test.tsNODE_OPTIONS=--max-old-space-size=4096 npm run typecheck:clinpm run build:cligit diff --checkTypeScript (CLI)andpackage.json ↔ git tag version syncpassed.Signed-off-by: ScarabSystems scarab.systems@yahoo.com
Disclosure: This PR was prepared with AI assistance under human direction and review.
Summary by CodeRabbit